Skip to content

Make ClearColorConfig::None work correctly (esp in the presence of MSAA)#23844

Merged
alice-i-cecile merged 7 commits intobevyengine:mainfrom
tychedelia:view-target-fixes
Apr 17, 2026
Merged

Make ClearColorConfig::None work correctly (esp in the presence of MSAA)#23844
alice-i-cecile merged 7 commits intobevyengine:mainfrom
tychedelia:view-target-fixes

Conversation

@tychedelia
Copy link
Copy Markdown
Member

Two separate issues:

  1. Because we were re-creating the atomics used for tracking which main texture is "active", post-process writes could be lost across frames because the atomic was being reset. This is a bug even without MSAA.
  2. We need to force writeback when the user opts into Load semantics on their camera and MSAA is enabled, otherwise we'll throw away the main target texture incorrectly.

@tychedelia tychedelia added C-Bug An unexpected or incorrect behavior A-Rendering Drawing game state to the screen S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Apr 17, 2026
@github-project-automation github-project-automation Bot moved this to Needs SME Triage in Rendering Apr 17, 2026
@tychedelia tychedelia added this to the 0.19 milestone Apr 17, 2026
@tychedelia
Copy link
Copy Markdown
Member Author

@beicause Some more here about MSAA writeback, I know you've had opinions here before.

Comment thread crates/bevy_render/src/view/mod.rs Outdated
Comment on lines +1245 to +1254
// re-use the same atomics frame to frame for views with the same main texture
// to ensure post process writes persist through msaa writeback
let main_texture = main_texture_atomics
.get(&key)
.and_then(Weak::upgrade)
.unwrap_or_else(|| {
let arc = Arc::new(AtomicUsize::new(0));
main_texture_atomics.insert(key.clone(), Arc::downgrade(&arc));
arc
});
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can use HashMap.entry

Is it correct to use Weak<AtomicUsize>, won’t it be dropped at the start of each frame?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically, we re-insert the ViewTarget every frame. As long as the configuration doesn't change, this is just replacing the effective same component over and over again. So there is an existing strong handle on the component, which we will replace here. I forgot to add the clean-up at the top of the system, but it's basically a cheap way to see if an entry is actually being used. I also just switched to using the actual entry api here, thanks!

@alice-i-cecile alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Apr 17, 2026
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Apr 17, 2026
Merged via the queue into bevyengine:main with commit 9f17c2e Apr 17, 2026
40 checks passed
@github-project-automation github-project-automation Bot moved this from Needs SME Triage to Done in Rendering Apr 17, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-Rendering Drawing game state to the screen C-Bug An unexpected or incorrect behavior S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants